Repository navigation
Conversation
The inputFlags/outputFlags/localFlags/controlFlags setters ran tcgetattr/tcsetattr on the PTY master and dropped both results, so an assignment the PTY rejected (EINVAL from glibc when the kernel refuses the requested c_cflag bits, ENOTTY/EBADF when the fd is no longer a tty) returned as if it had been applied. A setter's return value is not observable from JS, so throwing is the only way to report it, and it is what resize() and setRawMode() already do. get_termios/set_termios now return bun_sys::Result and the setter throws the SystemError when either call fails. Reading still yields 0 on failure and assigning on a closed terminal is still a no-op; the closed check moves after the value coercion so a valueOf() that closes the terminal takes the no-op path rather than throwing EBADF. Clears the error_collapsed_to_bool entry for Terminal.rs in mordant-baseline.toml.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
WalkthroughChangesThe Terminal termios helpers now preserve system errors. Flag setters throw on PTY failures, remain no-ops after closure, and retain existing clamping behavior. Type documentation and POSIX regression tests describe and verify these semantics. Terminal termios behavior
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:07 PM PT - Aug 15th, 2026
🔄 @robobun, the build for your commit |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — stops swallowing tcsetattr/tcgetattr failures in the termios flag setters and surfaces them as SystemError, matching resize()/setRawMode()/write().
What was reviewed:
set_termios_flag: coercion now precedes the closed check so avalueOf()that closes the terminal no-ops instead of hitting the new throw on an invalid fd; getter still returns 0 on failure and closed-terminal set is still a no-op (both pinned by existing tests).- Verified empirically on glibc 2.39 that clearing CREAD returns EINVAL (new gated test holds) while
c_cflag = tcflag_t::MAXsucceeds, so the updated "NaN clamps" loop'scontrolFlags = Infinitydoes not throw. get_termios/set_termioshave no other callers;set_raw_modeandcreate_pty_posixgo throughsys::posixdirectly and are unaffected.- Baseline entry removal is the fixed lint only; the unrelated
reimplemented_helperentry for Terminal.rs stays.
Extended reasoning...
Overview
Four files: src/runtime/api/bun/Terminal.rs changes get_termios/set_termios to return sys::Result and makes set_termios_flag throw the bun_sys::Error via SysErrorJsc::throw when either syscall fails; the closed-terminal check moves after coerce_f64 so a valueOf() that closes the terminal takes the existing no-op path. packages/bun-types/bun.d.ts corrects four JSDoc lines (the old "returns false on failure" was never observable — JSC discards CustomAccessor setter return values). mordant-baseline.toml drops the now-fixed error_collapsed_to_bool entry. test/js/bun/terminal/terminal.test.ts adds three tests and hardens the NaN-clamp test.
Security risks
None. No untrusted-input parsing, no auth/crypto, no allocation sizing changes. The only new user-observable path is a thrown SystemError where a syscall error was previously discarded.
Level of scrutiny
Low-medium. The Rust diff is ~15 lines of mechanical error propagation in a single helper with two call sites, both in this file. This is a user-facing behavior change (setter now throws on an ioctl failure it previously swallowed), but the PR argues convincingly that (a) the old behavior was a lint-flagged bug directly contrary to REVIEW.md's "never swallow a failure", (b) the documented "returns false" contract was never observable, and (c) sibling operations on the same object already throw on their ioctl failures. The failure conditions are edge cases: glibc's c_cflag readback rejection, or the master fd being invalidated behind the terminal's back.
Other factors
- Empirically confirmed on glibc 2.39 that
tcsetattrwith CREAD cleared on a PTY master fails with EINVAL (validating theisGlibc-gated test) whilec_cflag = 0xFFFFFFFFsucceeds — so the "NaN clamps" test'scontrolFlags = Infinityiteration does not throw under the new error propagation. The finder's concern about this readback being removed in newer glibc was examined and refuted by the verifier; the portable ENOTTY subprocess test covers all POSIX platforms regardless. - The ENOTTY test relies on POSIX lowest-free-fd semantics to swap
/dev/nullunder the master fd; it assertsswapped: trueso a violated assumption fails loudly rather than passing vacuously, and it runs in a fresh subprocess with a warmed fd table. - Checked that
get_termios/set_termioshave no other callers;set_raw_modeusesbun_core::tty::State::set_modeandcreate_pty_posixcallssys::posix::tcsetattrdirectly, so the signature change is fully contained. - The coerce-before-closed-check reordering does mean coercion side effects now run on an already-closed terminal where they previously didn't, but that is the conventional order (validate arguments before state) and is what makes the new test's
valueOf()-closes case take the no-op path instead of the new throw.
|
Correction to the comment below: the EINVAL readback is not upstream glibc behavior. It comes from the patch Debian and Ubuntu apply to their glibc (upstream Original commentNothing to change from this review. One data point worth recording for the glibc-gated test: the CREAD readback returning EINVAL was measured here on glibc 2.41 (Debian 13) and in the review on 2.39, and in the first CI run it held on every glibc lane (Debian 13 x64, x64-asan and aarch64, Ubuntu 25.04 x64 and aarch64). If a future glibc dropped the readback, that test would fail loudly on the affected lane rather than pass vacuously; the |
Upstream glibc's tcsetattr is a single TCSETS ioctl; the EINVAL for a c_cflag change the pty driver refuses comes from the patch Debian and Ubuntu apply to their glibc, so the isGlibc gate would fail the test on other glibc distributions. The /dev/null ENOTTY test covers the throw on every libc.
There was a problem hiding this comment.
LGTM — the earlier feedback was addressed (commit 2a8bc0d drops the isGlibc-gated EINVAL test and the remaining comment now correctly attributes the readback to Debian/Ubuntu's patch).
What was reviewed:
set_termios_flag: coercion now runs before the closed check, so avalueOf()that closes the terminal still no-ops instead of reachingget_termioson an invalid fd — pinned by the new test.- The portable ENOTTY test self-validates its fd-reuse precondition via
swapped: true, so a wrong fd-table assumption fails loudly rather than passing vacuously. get_termios_flag(the getter) still returns 0 on error, so the existing "flags return 0 on closed terminal" contract andflagsAfterSwap: 0both hold.- The added
terminal[prop] = originalin the NaN test only inserts an extra assignment on a path that was already green on Debian CI; it cannot introduce a new EINVAL.
Extended reasoning...
Overview
Four files: src/runtime/api/bun/Terminal.rs (~15 lines in set_termios_flag / get_termios / set_termios), test/js/bun/terminal/terminal.test.ts (two new tests, one modified), packages/bun-types/bun.d.ts (four JSDoc lines), and one line removed from mordant-baseline.toml. The Rust change makes the four termios flag setters throw the underlying SystemError when tcgetattr/tcsetattr fail, matching resize()/setRawMode()/write() on the same object, and reorders the closed-flag check to run after value coercion.
Prior feedback resolution
My previous review flagged the test.skipIf(!isGlibc) EINVAL test as depending on Debian's local-tcsetaddr.diff rather than upstream glibc. Commit 2a8bc0d removed that test entirely, dropped the isGlibc import, and updated the surviving comment in the NaN-clamp test to say "Debian's and Ubuntu's patched glibc" instead of attributing the readback to glibc generally. The portable /dev/null ENOTTY test remains as the throw-path coverage on every POSIX libc.
Security risks
None. The change surfaces an error that was previously swallowed; no new input is parsed and no trust boundary is crossed. The fd-swap test runs in an isolated subprocess.
Level of scrutiny
Low-to-medium. The Rust change is a small, mechanical error-propagation refactor in a Bun-native API (not a Node compat surface), following the same SysErrorJsc::throw pattern already used by write() a few lines below. The one non-mechanical piece — moving the closed check after coerce_f64 — is explained in-line and pinned by a dedicated test. The mordant baseline entry removal is exactly what the ratchet requires once the finding is fixed.
Other factors
- The NaN-clamp test's
terminal[prop] = InfinityforcontrolFlagswas already present on main and green on Debian CI, so inserting a restore-to-original between the two assignments cannot introduce a Debian-patch EINVAL that wasn't already there. - The ENOTTY subprocess test's fd-number-reuse trick is intricate, but it asserts
swapped: trueandhadFlagsBeforeSwap: truebefore checking the throw, so a broken precondition produces a clear failure rather than a vacuous pass. The throwawaynew Bun.Terminal({}).close()up front handles the lazydlopen("libutil")on first construction. bun.d.tswording is now accurate: JSC ignores a CustomAccessor setter's return value, so the old "returns false on failure" was never observable.- Bug-hunting system found nothing on this revision.
|
Status for whoever picks this up: the code is final at 49b6448.
|
Termios ioctls on /dev/null fail with ENODEV on macOS; a regular file fails with ENOTTY on every POSIX kernel, which is what the test asserts.
There was a problem hiding this comment.
LGTM — the earlier isGlibc-gated test was dropped in 2a8bc0d as requested, and 49b6448 switched the ENOTTY fixture from /dev/null to a regular file for macOS.
What was reviewed:
set_termios_flagnow propagatestcgetattr/tcsetattrerrors viaSysErrorJsc::throw(matchingwrite()/resize()/setRawMode()); the getter still returns 0 on failure, and the closed check moved aftercoerce_f64so avalueOf()that closes the terminal takes the no-op path.- Checked that
get_termios/set_termioshave no other callers (create_pty_posixusessys::posix::tcgetattr/tcsetattrdirectly), so the signature change is self-contained. - The ENOTTY subprocess test asserts
swapped: trueso a fixture-level fd-reuse miss is distinguishable from a setter regression; the NaN test's restore-between-assignments avoids Debian's patched-glibc EINVAL.
Extended reasoning...
Overview
Clears one error_collapsed_to_bool mordant finding in src/runtime/api/bun/Terminal.rs by having get_termios/set_termios return sys::Result and having the four termios flag setters (inputFlags/outputFlags/localFlags/controlFlags) throw the bun_sys::Error as a SystemError instead of discarding it. The closed-terminal check is reordered to run after coerce_f64 so a valueOf() that closes the terminal hits the no-op path rather than the new throw. The matching baseline entry is removed, four JSDoc lines in bun.d.ts are updated (the old "returns false" wording described nothing observable — JSC ignores CustomAccessor setter returns), and terminal.test.ts gains two tests plus a restore step in the existing NaN test.
Security risks
None. The change surfaces an already-occurring syscall error to JS instead of swallowing it; no new untrusted-input parsing, no fd or handle lifetime changes.
Level of scrutiny
Low-to-moderate. The native diff is ~15 lines in one function and two trivial helper signature changes with no other callers. It is a user-visible behavior change (setter now throws where it silently returned), but it aligns with sibling methods on the same object (resize(), setRawMode(), write() all already throw on ioctl failure) and with REVIEW.md's "never swallow a failure" rule; the old behavior was documented as something that could never actually be observed.
Other factors
My earlier review flagged the isGlibc-gated EINVAL test as too broad (Debian patch, not upstream glibc); the author confirmed against upstream tcsetattr.c and dropped it in 2a8bc0d, keeping the portable ENOTTY test. 49b6448 then switched the ENOTTY fixture from /dev/null to a regular file after the darwin lane returned ENODEV. Per the author's status comment, 2a8bc0d was green on all Linux (Debian/Ubuntu/Alpine, x64/aarch64/ASAN), Windows, and darwin lanes except an unrelated test/bake/deinitialization.test.ts segfault on Windows 2019 that also fails on main. mordant and cargo clippy checks pass. The remaining review thread is resolved.
|
#42589 edits the same function, If #42589 lands first, this PR keeps its
#42589 also rewrites the JSDoc sentence "Setting returns true on success, false on failure", so that hunk needs a manual merge. The clause "throws if the PTY rejects the new flags" from this PR still has to be added there. |
Problem
bun run rust:mordantreports oneerror_collapsed_to_boolinsrc/runtime/api/bun/Terminal.rs:set_termiosturns thebun_sys::Errorfromtcsetattrintofalse, andset_termios_flag(the shared body of theinputFlags/outputFlags/localFlags/controlFlagssetters, line 1410 on main) ignores it. Thetcgetattrside had the same shape:get_termiosmapped its error toNoneand the setter returned early.controlFlagsrequest whose only changes are bits the pty driver forces (CREAD, CS8, no PARENB) fails with EINVAL. Upstream glibc'stcsetattris a single ioctl, so this part is distribution specific; bun's Linux glibc CI lanes are all Debian/Ubuntu.resize()andsetRawMode()on the same object already throw when their ioctl fails, andwrite()throws theSystemError; node-pty'sresizethrows on a failed TIOCSWINSZ as well. A setter has no other channel: JSC ignores the return value of aCustomAccessorsetter (JSObject::putInlineSlow), and thebun_jscsetter thunk mapsOk(false)andOk(true)to the same thing, so the "returns false on failure" wording in the bun-types JSDoc never described anything observable.Fix
get_termios/set_termiosreturnbun_sys::Result; the setter throws the error as aSystemError(code,syscall: "ioctl",errno, same conversionwrite()uses) when either call fails. The getter still reads 0 on failure, and assigning on a closed terminal is still a no-op (both pinned by existing tests).valueOf()that closes the terminal takes the no-op path instead of reaching the new throw with an invalid fd. Before this change that window was covered by the error being swallowed.mordant-baseline.toml: theerror_collapsed_to_boolentry for Terminal.rs is deleted by hand. A fullbun run rust:mordant:baselineregeneration also droppedalways_unwrapped_option:src/install/PackageInstall.rs(already removed in install: rename with_alias_of's version parameter and clear its mordant baseline entry #39130) andnarrowed_two_ways:src/runtime/node/node_crypto_binding.rs, which are unrelated, so they are left in place here. Thereimplemented_helperentry for Terminal.rs is a different site (get_optional_value, line 243) and stays.test/js/bun/terminal/terminal.test.ts, "termios flags" block:setting flags throws when the terminal's fd is not a PTY(any POSIX): a child process swaps the master fd for a regular file behind the terminal's back; the getter reads 0 and the assignment throws ENOTTY. No libc rejects a termios update on a healthy PTY portably, so this is the deterministic way to reach the throw. (A regular file rather than/dev/null: termios ioctls on/dev/nullare ENODEV on macOS, which the first darwin run showed; the throw itself behaved the same there.)setting flags on a terminal closed while coercing the value is a no-op: pins the check order above.NaN clamps to tcflag_t maxnow restores the original flags between the Infinity and NaN assignments: on the Debian/Ubuntu lanes, assigning the same partly-maskedcontrolFlagsvalue twice is exactly the EINVAL case above, and starting both assignments from the same state is also what makes the comparison meaningful.USE_SYSTEM_BUN=1, assignment silently returns) and passes withbun bd;bun bd test test/js/bun/terminal/ test/js/node/tty.test.ts: all pass.bun run rust:mordantwith this baseline: without thesrc/change it reports the Terminal.rs:1410 finding (bun_runtime 1intarget/mordant/over-baseline.txt); with it, nothing over the baseline. Themordantandcargo clippyGitHub checks passed on the first push as well.Background
c_iflag,c_oflag,c_lflag,c_cflag) are whatBun.Terminalexposes as the four properties. A setter doestcgetattron the PTY master, replaces one word, and writes the struct back withtcsetattr(TCSANOW);bun_sys::posix::tcgetattr/tcsetattrreturnResult<_, bun_sys::Error>tagged as theioctlsyscall.CS8 | CREADset and parity off regardless of what is written, and the kernel reports success. Debian's glibc packaging (inherited by Ubuntu) patchestcsetattrto read the attributes back and return EINVAL when the requestedc_cflagchange had no effect; upstream glibc (sysdeps/unix/sysv/linux/tcsetattr.cis oneioctl(TCSETS)), musl, bionic and macOS return the kernel's success. That is why there is no direct PTY-based test for the EINVAL case and the test uses a regular file's fd instead (every POSIX kernel answers termios ioctls on a regular file with ENOTTY).SysErrorJsc::throwconverts abun_sys::Errorinto the JSSystemError(codesuch as"ENOTTY",syscall,errno) and throws it; it is the same conversion Terminal'swrite()and mostnode:fspaths use.mordant-baseline.tomlis a ratchet: it records the accepted finding count per (lint, file), and CI fails only on findings above the recorded count. Once a finding is fixed its entry has to go, otherwise it would hide a new finding of the same lint in that file.Earlier revision of this PR
The first push also had a
test.skipIf(!isGlibc)test asserting that clearing CREAD throws EINVAL, attributing the readback to glibc itself. Review pointed out the readback is Debian's patch, not upstream glibc, so theisGlibcgate would fail the test on Fedora, Arch, Amazon Linux and similar; confirmed against upstreamtcsetattr.cand dropped in 2a8bc0d. The ENOTTY test carries the behavior on every libc; 49b6448 switched its stand-in fd from/dev/nullto a regular file after the darwin lane reported ENODEV for/dev/null.no test proof · iteration 5 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/terminal/terminal.test.ts